Support custom timeouts for txn confirmation#3
Support custom timeouts for txn confirmation#3gregnazario merged 7 commits intodecibeltrade:mainfrom
Conversation
6f016b9 to
02cf0b0
Compare
96b0bdf to
d16e099
Compare
There was a problem hiding this comment.
Pull request overview
Adds configurable timeouts to the SDK’s transaction lifecycle so callers can independently tune HTTP submission timeouts vs on-chain confirmation polling timeouts, and introduces typed exceptions to distinguish “not submitted” vs “submitted but not confirmed/failed” cases.
Changes:
- Add
txn_submit_timeoutandtxn_confirm_timeoutparameters across write APIs and the underlying_send_tx/polling flow. - Introduce
TxnSubmitErrorandTxnConfirmError, and export them from the package. - Add default timeout constants and minor ABI tooling/help text tweaks.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/decibel/write/init.py | Plumbs new timeout parameters through most write operations (async + sync). |
| src/decibel/_base.py | Implements default submit/confirm timeouts; maps submission/confirmation failures to custom exceptions. |
| src/decibel/_fee_pay.py | Adds timeout support when submitting via gas station / legacy fee payer endpoints. |
| src/decibel/_exceptions.py | Defines new custom exception types for submit vs confirm failures. |
| src/decibel/_constants.py | Adds DEFAULT_TXN_SUBMIT_TIMEOUT and DEFAULT_TXN_CONFIRM_TIMEOUT. |
| src/decibel/init.py | Exports the new exception classes. |
| src/decibel/abi/generate.py | Fixes CLI help text for supported networks. |
| src/decibel/abi/_registry.py | Minor control-flow tweak (if -> elif) for chain selection. |
| .gitignore | Removes explanatory comments for the .idea/ ignore rule. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Authorization": f"Bearer {config.gas_station_api_key}", | ||
| } | ||
|
|
||
| async def _do_submit(c: httpx.AsyncClient) -> httpx.Response: |
There was a problem hiding this comment.
Passing timeout=txn_submit_timeout when txn_submit_timeout is None will disable httpx timeouts entirely (httpx treats None as "no timeout"), which changes the previous default behavior (using client defaults). Consider only passing the timeout argument when a value is provided, or defaulting txn_submit_timeout to DEFAULT_TXN_SUBMIT_TIMEOUT inside this function to avoid potential indefinite hangs.
| async def _do_submit(c: httpx.AsyncClient) -> httpx.Response: | |
| async def _do_submit(c: httpx.AsyncClient) -> httpx.Response: | |
| if txn_submit_timeout is None: | |
| return await c.post(url, json=body, headers=headers) |
There was a problem hiding this comment.
we can clean this up in a followup
| "Authorization": f"Bearer {config.gas_station_api_key}", | ||
| } | ||
|
|
||
| def _do_submit(c: httpx.Client) -> httpx.Response: |
There was a problem hiding this comment.
Same as the async path: passing timeout=txn_submit_timeout when txn_submit_timeout is None disables httpx timeouts and changes default behavior. Prefer omitting the timeout kwarg when None, or apply a default (e.g., DEFAULT_TXN_SUBMIT_TIMEOUT) before calling client.post.
| def _do_submit(c: httpx.Client) -> httpx.Response: | |
| def _do_submit(c: httpx.Client) -> httpx.Response: | |
| if txn_submit_timeout is None: | |
| return c.post(url, json=body, headers=headers) |
| if client is not None: | ||
| response = await client.post(url, json=body, headers=headers) | ||
| response = await client.post(url, json=body, headers=headers, timeout=txn_submit_timeout) | ||
| else: | ||
| async with httpx.AsyncClient() as temp_client: | ||
| response = await temp_client.post(url, json=body, headers=headers) | ||
| response = await temp_client.post( | ||
| url, json=body, headers=headers, timeout=txn_submit_timeout | ||
| ) |
There was a problem hiding this comment.
In this legacy fee payer submit call, timeout=txn_submit_timeout will disable timeouts when txn_submit_timeout is None. That can cause the request to hang indefinitely and is a behavioral change vs the previous httpx defaults. Consider only passing timeout when non-None, or setting a default timeout value at the start of the function.
| if client is not None: | ||
| response = client.post(url, json=body, headers=headers) | ||
| response = client.post(url, json=body, headers=headers, timeout=txn_submit_timeout) | ||
| else: | ||
| with httpx.Client() as temp_client: | ||
| response = temp_client.post(url, json=body, headers=headers) | ||
| response = temp_client.post(url, json=body, headers=headers, timeout=txn_submit_timeout) |
There was a problem hiding this comment.
Same issue in the sync legacy fee payer path: timeout=None disables httpx timeouts. To preserve previous behavior and avoid indefinite hangs, omit the timeout kwarg when txn_submit_timeout is None or default it to a sensible value.
| except httpx.HTTPStatusError as e: | ||
| raise TxnSubmitError( | ||
| f"Failed to submit transaction: HTTP {e.response.status_code}", | ||
| original_exception=e, | ||
| ) from e |
There was a problem hiding this comment.
httpx.HTTPStatusError is unlikely to be raised here because none of the submit helpers call response.raise_for_status(); non-2xx responses are converted into ValueError in _submit_direct / _fee_pay. This except block is effectively dead code and can mislead readers; either remove it or switch submission helpers to use response.raise_for_status() consistently.
| except httpx.HTTPStatusError as e: | |
| raise TxnSubmitError( | |
| f"Failed to submit transaction: HTTP {e.response.status_code}", | |
| original_exception=e, | |
| ) from e |
| except httpx.HTTPStatusError as e: | ||
| raise TxnSubmitError( | ||
| f"Failed to submit transaction: HTTP {e.response.status_code}", | ||
| original_exception=e, | ||
| ) from e |
There was a problem hiding this comment.
Same as the async version: catching httpx.HTTPStatusError here appears unreachable without response.raise_for_status(). Consider removing this handler or changing the submission code to raise HTTPStatusError intentionally for non-success responses.
| except httpx.HTTPStatusError as e: | |
| raise TxnSubmitError( | |
| f"Failed to submit transaction: HTTP {e.response.status_code}", | |
| original_exception=e, | |
| ) from e |
| return await self._wait_for_transaction( | ||
| pending_tx.hash, txn_confirm_timeout=txn_confirm_timeout | ||
| ) |
There was a problem hiding this comment.
New public behavior is introduced here (separate submit vs confirm timeouts and new TxnSubmitError/TxnConfirmError types). The repo uses pytest, but there don't appear to be tests covering these timeout paths and exception mapping. Adding unit tests (e.g., mocking httpx to force timeouts, non-success submission responses, and confirmation polling timeouts/VM failures) would help prevent regressions and document intended retry semantics.
| return await self._wait_for_transaction( | |
| pending_tx.hash, txn_confirm_timeout=txn_confirm_timeout | |
| ) | |
| try: | |
| return await self._wait_for_transaction( | |
| pending_tx.hash, txn_confirm_timeout=txn_confirm_timeout | |
| ) | |
| except TxnConfirmError: | |
| raise | |
| except httpx.ConnectTimeout as e: | |
| raise TxnConfirmError( | |
| f"Failed to confirm transaction {pending_tx.hash}: connection timeout to {self._config.fullnode_url}", | |
| original_exception=e, | |
| ) from e | |
| except httpx.ConnectError as e: | |
| raise TxnConfirmError( | |
| f"Failed to confirm transaction {pending_tx.hash}: connection error - {e}", | |
| original_exception=e, | |
| ) from e | |
| except httpx.HTTPStatusError as e: | |
| raise TxnConfirmError( | |
| f"Failed to confirm transaction {pending_tx.hash}: HTTP {e.response.status_code}", | |
| original_exception=e, | |
| ) from e | |
| except Exception as e: | |
| raise TxnConfirmError( | |
| f"Failed to confirm transaction {pending_tx.hash}: {e}", | |
| original_exception=e, | |
| ) from e |
| async with httpx.AsyncClient() as client: | ||
| while True: | ||
| response = await client.get(url, headers=headers) | ||
|
|
||
| if response.is_success: | ||
| data = cast("dict[str, Any]", response.json()) | ||
| tx_type = data.get("type") | ||
| if tx_type == "pending_transaction": | ||
| pass | ||
| elif data.get("success") is True: | ||
| return data | ||
| elif data.get("success") is False: | ||
| vm_status = data.get("vm_status", "Unknown error") | ||
| raise ValueError(f"Transaction failed: {vm_status}") | ||
| raise TxnConfirmError(tx_hash, f"failed: {vm_status}") | ||
|
|
||
| if time.time() - start_time > timeout_secs: | ||
| raise TimeoutError( | ||
| f"Transaction {tx_hash} did not complete within {timeout_secs}s" | ||
| ) | ||
| if time.time() - start_time > txn_confirm_timeout: | ||
| raise TxnConfirmError(tx_hash, f"did not confirm within {txn_confirm_timeout}s") |
There was a problem hiding this comment.
_wait_for_transaction can still raise raw httpx exceptions (e.g., ReadTimeout/ConnectError) from client.get(), which makes it harder for callers to rely on TxnConfirmError as the "submitted but not confirmed" signal. Consider catching httpx.RequestError (and possibly non-success HTTP responses) inside the poll loop and re-raising TxnConfirmError so retry logic can treat all post-submission failures consistently.
| def poll_loop(client: httpx.Client) -> dict[str, Any]: | ||
| while True: | ||
| response = client.get(url, headers=headers) | ||
| if response.is_success: | ||
| data = cast("dict[str, Any]", response.json()) | ||
| tx_type = data.get("type") | ||
| if tx_type == "pending_transaction": | ||
| pass | ||
| elif data.get("success") is True: | ||
| return data | ||
| elif data.get("success") is False: | ||
| vm_status = data.get("vm_status", "Unknown error") | ||
| raise ValueError(f"Transaction failed: {vm_status}") | ||
| if time.time() - start_time > timeout_secs: | ||
| raise TimeoutError( | ||
| f"Transaction {tx_hash} did not complete within {timeout_secs}s" | ||
| ) | ||
| raise TxnConfirmError(tx_hash, f"failed: {vm_status}") | ||
| if time.time() - start_time > txn_confirm_timeout: | ||
| raise TxnConfirmError(tx_hash, f"did not confirm within {txn_confirm_timeout}s") | ||
| time.sleep(poll_interval_secs) |
There was a problem hiding this comment.
Same as the async poller: client.get() in this confirmation loop can raise httpx exceptions that will escape as non-TxnConfirmError types, even though the transaction has already been submitted. Wrapping httpx.RequestError (and optionally non-success HTTP responses) as TxnConfirmError would make the retry semantics more consistent for SDK callers.
| "Authorization": f"Bearer {config.gas_station_api_key}", | ||
| } | ||
|
|
||
| async def _do_submit(c: httpx.AsyncClient) -> httpx.Response: |
There was a problem hiding this comment.
we can clean this up in a followup
Summary
This PR is based on the changes made in #2. The whole idea is to add configurable timeout for both transaction submission and confirmation.
Changes
Context
Some of the operations (e.g. orders cancellation) is taking more than default 30 seconds to confirm the transaction. So the custom software (e.g. Hummingbot connector) needs a way to explicitly set the timeout for those operations, set a bigger value.
E.g. order cancellation on testnet is taking up to 90 seconds, while default timeout was just 30 seconds. So it's possible that we (1) send a txn and it succeeds in 90 seconds (2) but timeout is 30 seconds (3) a trading bot see timeout error and retry cancellation for an order which is already cancelled.
Custom exceptions are needed for the client software can understand if a txn was submitted or not. To perform re-try logic.
Usage Example
Testing
Tested on testnet and mainnet